Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hidden --preview / --no-preview options to ruff check #7009

Merged
merged 7 commits into from
Aug 31, 2023
Merged

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Aug 30, 2023

Per discussion at #6998

Summary

Adds a --preview and --no-preview option to the CLI for ruff check and corresponding settings. The CLI options are hidden for now.

Available in the settings as preview = true or preview = false.

Does not include environment variable configuration, although we may add it in the future.

Test Plan

cargo build

Future work will build on this setting, such as toggling the mode during a test.

@charliermarsh charliermarsh added the cli Related to the command-line interface label Aug 30, 2023
@MichaReiser
Copy link
Member

Should we introduce a PreviewMode enum that has Enabled and Disabled variant to make it easier to search for preview usages (instead of having to search for booleans named preview)?

@charliermarsh
Copy link
Member

@MichaReiser - That's a nice idea.

@zanieb
Copy link
Member Author

zanieb commented Aug 30, 2023

I like it

@github-actions
Copy link
Contributor

github-actions bot commented Aug 30, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

)]
/// Whether to enable preview mode. When preview mode is enabled, Ruff will
/// use unstable rules and fixes.
pub preview: Option<bool>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So.. I think we could make this Option<PreviewMode> too but I'm not entirely sure how to make that play nicely with the JsonSchema. I'm also not sure if we want preview = enabled | disabled or preview = true | false for users.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I quickly looked into this but don't see a good solution for it other than implementing Serialize, Deserialize and JsonSchema manually on PreviewMode if we want preview = true | false (preview = enabled | disabled) should work out of the box.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'm happy to leave it as-is then

Comment on lines 104 to 108
if version {
PreviewMode::Enabled
} else {
PreviewMode::Disabled
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This always reminds me of @charliermarsh's tweet about how high-performance code looks like 😆

Comment on lines 96 to 100
pub enum PreviewMode {
Enabled,
#[default]
Disabled,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type being inside of the ruff crate means that we can't use it in the formatter, or any other crate (e.g. consider we need to gate some semantic model change)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm it seems wrong to put it elsewhere right now since there's a dedicated settings/types.rs module. This is a good callout though, we should probably have a ruff_settings crate so we can share with the formatter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just move the PreviewMode because the type itself isn't specific to settings.

A ruff_settings crate isn't straightforward because the settings depend on the rules. So you would need to make the formatter depend on ruff.

I consider (at least most of what is in settings) settings as the linter-specific settings. The formatter has its own resolved settings that only contain the formatter settings. Most of this already exists with PyFormatOptions, although we probably want to add a few more settings.

What I have in mind is:

  • each tool has its own resolved settings object
  • Configuration is the unified configuration from which the workspace derives the tool Settings

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where would you expect it to go?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know 😅

It would need to be some rather low-level crate that other crates can depend on without pulling in too many dependencies.

The alternative is to duplicate the enum in project where it is needed. I don't really mind that (e.g. the Linter has LineLength and the formatter has a very similar LineWidth option type)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright let's consider this TBD when preview support is added to the formatter

Copy link
Member

@MichaReiser MichaReiser Aug 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another way we could take is that PreviewMode remains in the configuration only and setting it to true or false changes individual settings in the tool-specific settings. Each tool can then decide whether it uses feature-specific flags or a single preview flag. For example, the formatter can either:

  • Have a single preview setting that is set to enabled/disabled based on the preview mode
  • Or an experimental-string-handling and potentially other settings that configure a specific preview feature. Setting preview=true would enable/disable all of them at once.

The benefit of feature specific flags is that individual features can be moved out of preview mode (and makes it easier to find all places where the check now needs to be removed)

@zanieb zanieb merged commit 96a9717 into main Aug 31, 2023
@zanieb zanieb deleted the cli/preview branch August 31, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command-line interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants